-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 674 gcp logging spaninfo #677
base: main
Are you sure you want to change the base?
Conversation
I'm not convinced about the labels, do they bring any added value? I understand that logback do this but for which purpose? |
There was one general and one more specific reason I added the labels:
In short, I appreciate that there isn't a compelling technical reason for these labels, just a minor-ish usage one. If you'd prefer me to remove them, that's fine. |
Pinging @Fungrim as he was the original contributor of Google Cloud Logging extension, what do you think about the proposed default labels, does it makes sense? |
if it helps I can split out the labels work into a separate PR, and leave this PR to focus on the tracing fix, as the latter is I think of far greater importance. |
Thanks for the ping @loicmathieu, and the span is a nice catch @wabrit. Regarding the labels and don't have any strong personal opinions. However, given the lack of standardization around log records and formats, I generally dislike hard coded values. If I did this change myself I would probably add a But again, these are not strong so I'm fine with the proposed changes as well. |
The I'm happy to go either of the following ways:
@loicmathieu let me know what you would prefer (FWIW my preference would be option (2) as I'd really like to get the trace fix in). |
These changes address issue 674 by ensuring that the span id and sampled properties are set on the
LogEntry.builder
(without these settings it is not possible to exploit the feature of GCP console where a log entry can be used to launch inspection of the trace corresponding to that log (and vice versa).In addition the following labels are added:
levelName
: the original level name of the underlyingExtLogRecord
levelValue
: the original level numeric value of the underlyingExtLogRecord
loggerName
: the source class name of the underlyingExtLogRecord
(if present)as they can be useful, and to provide some consistency with the labels output by the
com.google.cloud:google-cloud-logging-logback
dependency used for support of GCP logging in logback.These changes have been tested in a native-build Quarkus microservice (running Quarkus 3.11.3).